Skip to content

feat: Add compute aggregation kernels (Sum/Min/Max/Mean)#379

Open
luisquintanilla wants to merge 5 commits into
apache:mainfrom
luisquintanilla:feat-compute-aggregation-kernels
Open

feat: Add compute aggregation kernels (Sum/Min/Max/Mean)#379
luisquintanilla wants to merge 5 commits into
apache:mainfrom
luisquintanilla:feat-compute-aggregation-kernels

Conversation

@luisquintanilla

Copy link
Copy Markdown

What's Changed

Adds a new Apache.Arrow.Compute project with SIMD-accelerated Sum/Min/Max/Mean aggregation kernels for PrimitiveArray<T>, backed by System.Numerics.Tensors (TensorPrimitives). When NullCount == 0 the kernels dispatch to TensorPrimitives for one vectorized pass over the contiguous Values span; with nulls they fall back to a validity-aware scalar loop. The project is AOT-compatible with scalar fallbacks for older target frameworks, and pulls in System.Numerics.Tensors via Directory.Packages.props.

Includes an xUnit test project (Apache.Arrow.Compute.Tests, 11 tests) covering int32, int64, float, and double, with and without nulls, and the empty-array case. The new projects are added to Apache.Arrow.sln and the test to Apache.Arrow.Tests.slnf.

This implements the aggregation-kernel portion of #375; the elementwise kernels and Tensor<T> wrapper described there remain as follow-ups.

Part of #375.

## What's Changed

Adds a new Apache.Arrow.Compute project with SIMD-accelerated Sum/Min/Max/Mean
aggregation kernels for PrimitiveArray<T>, backed by System.Numerics.Tensors
(TensorPrimitives). When NullCount == 0 the kernels dispatch to TensorPrimitives for
one vectorized pass over the contiguous Values span; with nulls they fall back to a
validity-aware scalar loop. The project is AOT-compatible with scalar fallbacks for
older target frameworks, and pulls in System.Numerics.Tensors via Directory.Packages.props.

Includes an xUnit test project (Apache.Arrow.Compute.Tests, 11 tests) covering int32,
int64, float, and double, with and without nulls, and the empty-array case. The new
projects are added to Apache.Arrow.sln and the test to Apache.Arrow.Tests.slnf.

This implements the aggregation-kernel portion of apache#375; elementwise kernels and the
Tensor<T> wrapper described there remain as follow-ups.

Part of apache#375.
@CurtHagenlocher

Copy link
Copy Markdown
Contributor

This overlaps a bit with #257, at least in spirit. FYI @mobiusklein.

@mobiusklein

Copy link
Copy Markdown
Contributor

I ran out of bandwidth. The scope of my PR was too large to work through in the time I had available. If @luisquintanilla wants to adapt any of that material, please feel free to. Otherwise, I won't be able to cycle back to it in the next month at least.

@luisquintanilla

Copy link
Copy Markdown
Author

Thanks folks. I'll take a look and see how to reconcile both PRs where possible.

To @CurtHagenlocher point, seems like they're both aiming to add operations to PrimitiveArray so looks like there's definitely opportunities.

## What's Changed

Makes the Apache.Arrow.Compute aggregation kernels build across the same target
frameworks as Apache.Arrow (netstandard2.0;net8.0;net462):

- On net8.0+ the kernels stay generic over INumber<T> and dispatch to
  System.Numerics.Tensors (TensorPrimitives) on the null-free fast path.
- On netstandard2.0/net462 (where generic math and TensorPrimitives are
  unavailable) they fall back to per-type scalar kernels for
  Int32Array/Int64Array/FloatArray/DoubleArray.
- Documents the LINQ-style null semantics (nulls are skipped; Sum of an empty or
  all-null array is zero; Min/Max/Mean throw InvalidOperationException when there
  are no non-null elements).

Adds an Apache.Arrow.Compute.Benchmarks project (BenchmarkDotNet) comparing the
SIMD fast path against a naive managed loop over the same buffer; the SIMD Sum is
several times faster at large sizes with zero allocation. Wires the benchmark
project into Apache.Arrow.sln.

Part of apache#375.
@luisquintanilla

Copy link
Copy Markdown
Author

Pushed an update that gets this PR closer to mergeable (keeping it as a draft for now):

  • Multi-targets netstandard2.0;net8.0;net462 to match Apache.Arrow. On net8.0+ the kernels stay generic over INumber<T> and dispatch to System.Numerics.Tensors (TensorPrimitives) on the null-free fast path; on netstandard2.0/net462 (where generic math and TensorPrimitives aren't available) they fall back to per-type scalar kernels for Int32Array/Int64Array/FloatArray/DoubleArray.
  • Null handling follows LINQ semantics (nulls skipped; Sum of empty/all-null is zero; Min/Max/Mean throw InvalidOperationException when there are no non-null elements), with a validity-aware loop when nulls are present.
  • Benchmarks added (Apache.Arrow.Compute.Benchmarks, BenchmarkDotNet). On a quick run the SIMD Sum is roughly 6-7x a naive managed loop over the same buffer at 1M elements, with zero allocation.
  • 11 unit tests, green on all target frameworks.

This is the first slice of #375; a roadmap for the follow-on slices (elementwise kernels, Tensor<T> interop, and an extended aggregation surface) is on that issue.

On the project home / naming. These are compute kernels in the sense of C++ arrow::compute and pyarrow.compute, so this PR puts them in a dedicated Apache.Arrow.Compute project. I noticed the Apache.Arrow.Operations name suggested on #254 is now used by the Arrow Variant binary-format codecs, so a separate Apache.Arrow.Compute keeps the numeric/array kernels cleanly apart from those codecs. Happy to align with whatever you'd prefer here, @CurtHagenlocher (including a different home for the kernels in #257).

@luisquintanilla

Copy link
Copy Markdown
Author

@CurtHagenlocher this one's ready for review. Thanks!

@adamreeve adamreeve left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @luisquintanilla, I've left a few minor comments.

To fix the CI tests, you'll need to update dev/release/verify_rc.ps1 and dev/release/verify_rc.sh to make the new test project reference the nuget packages. See here and here.

The documentation build is also failing, it's not immediately obvious to me how to fix that.

Comment thread Apache.Arrow.sln Outdated
Comment thread Apache.Arrow.sln
Comment thread src/Apache.Arrow.Compute/Aggregations.cs Outdated
Comment thread src/Apache.Arrow.Compute/Aggregations.cs Outdated
@luisquintanilla

Copy link
Copy Markdown
Author

Thanks for the review @adamreeve

I believe I've addressed all the issues and feedback.

@CurtHagenlocher

Copy link
Copy Markdown
Contributor
  • Null handling follows LINQ semantics (nulls skipped; Sum of empty/all-null is zero; Min/Max/Mean throw InvalidOperationException when there are no non-null elements), with a validity-aware loop when nulls are present.

I think some of these LINQ semantics would be surprising to most data folks. Is there a difficulty in typing Min / Max / Mean to return a Nullable<T> and having them return null when there are no non-null elements? Otherwise, it puts a burden on the caller to either do an expensive pre-scan or to incur the rather steep cost of trapping an exception.

On the project home / naming. These are compute kernels in the sense of C++ arrow::compute and pyarrow.compute, so this PR puts them in a dedicated Apache.Arrow.Compute project. I noticed the Apache.Arrow.Operations name suggested on #254 is now used by the Arrow Variant binary-format codecs, so a separate Apache.Arrow.Compute keeps the numeric/array kernels cleanly apart from those codecs. Happy to align with whatever you'd prefer here, @CurtHagenlocher (including a different home for the kernels in #257).

I don't have a super strong preference about this, other than to insist that if we have an additional assembly then there must be clear guidelines about how to decide between the two where functionality goes. I don't see a particularly clear line between "compute operations" and "non-compute operations" but I also haven't put very much thought into it.

@adamreeve

Copy link
Copy Markdown
Contributor

I think some of these LINQ semantics would be surprising to most data folks

FWIW, I checked how PyArrow's compute module behaves and it returns None in these scenarios. Making these return a Nullable would also make it more obvious to users that they need to handle this case. It's easier to forget to catch an exception.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants